Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement runner, API, and client code for Inquiries #3653

Merged
merged 139 commits into from
Oct 3, 2017
Merged

Conversation

Mierdin
Copy link
Member

@Mierdin Mierdin commented Aug 7, 2017

This PR introduces a new runner, inquirer, as well as API endpoints and st2client changes necessary for working with Inquiries.

Usage

Please review the corresponding st2docs PR for full usage documentation and examples.

asciicast

New "Inquiry" Runner

This PR creates the inquirer runner, which forms the basis for "asking a question" in the middle of a Workflow.

Based on previous and current (see below) design discussions, this runner is fairly simple:

  • Dispatch trigger indicating a new inquiry
  • Request that the parent execution (workflow) is paused (for nested workflows, this is done on the root workflow)
  • Return a pending status

The entire act of handling a response (including validation) and resuming the workflow is handled by the API, and is outside the scope of this runner. This runner's sole purpose is to pause the workflow and provide enough context for a 3rd party to make a proper response.

The inquirer runner supports a number of parameters with sensible defaults. These defaults can of course be overridden by passing them into the action upon invocation. The parameters used for an Inquiry will be placed in the action result.

Currently the ttl parameter does nothing, and will be used in a future PR where st2garbagecollector will use it to clean up old Inquiries

API

For the time being, Inquiries are effectively treated as ActionExecutions, with a bit of additional logic, meaning we haven't built a full data model for Inquiries yet. This actually works pretty well, but there are a few "weird" things in this PR, especially around RBAC, because of this decision.

For the time being, the API is (hopefully) designed under the assumption that Inquiries may be their own data model in the future, and acts like it is today.

This API will provide:

  • GET /api/v1/inquiries: Retrieve all Inquiries
  • GET /api/v1/inquiries/{id}: Retrieve a specific inquiry by ID
  • PUT /api/v1/inquiries/{id}: Provide response data to an Inquiry

st2client Updates

There are three new commands, one for each new API function:

  • st2 inquiry list
  • st2 inquiry get <id>
  • st2 inquiry respond <id> <response json>

I opted to keep command-line options to a minimum, as most of the options for the similar st2 executions list command are focused mainly on filtering, and Inquiries shouldn't be that long-lived. Inquiries (for now) are basically just executions with a certain status, and they shouldn't be in that status for long. So I added the limit option, but not much else. Let me know if you feel other filters, like datetime filters are required, but I'm thinking they shouldn't be necessary.

Testing Instructions

NOTE that this is not a complete picture of everything we want to do with Inquiries. This PR focused on an end-to-end implementation of the core functionality. There are a few other misc. things that need to be done to really round this feature out, like adding garbage collection and ensuring things work well with chatops. For this round of testing I want to focus on the core functionality, like the way the API and CLI work, and the invocation of the action

This is a large and significant feature so manual testing before the merge is a good idea. Here are some things to test

Please first review the corresponding st2docs PR containing formal descriptions of what Inquiries are and how to use them (and please leave comments on that PR as needed too)

The following commands will check this branch out, and spin up a development instance of st2, with mistral, and install the client:

git clone -branch api-ask-response git@github.com:StackStorm/st2.git
cd st2
make requirements
source virtualenv/bin/activate
tools/launchdev.sh stop && tools/launchdev.sh startclean -m -x && python st2client/setup.py install > /dev/null

From there, you can create and execute workflows and rules that test Inquiries. Note the existing Mistral and ActionChain workflows in this branch that may serve as a good place to start.

There are a number of things that I feel would be useful to test, but by all means, go beyond this list:

  • Basic testing. Run a simple workflow with an Inquiry (core.ask action), and respond to it. Confirm that the workflow initially pauses, then respond to the inquiry. Then confirm the workflow resumes. Also confirm that the response fails with invalid data (and workflow does not resume)
  • Test with various parameters. Each parameter has an assumed default, override these and confirm that the behavior is as expected. For instance, override the schema parameter with your own, and supply data that would only validate against that
  • Nested workflows. Confirm that pauses and resumes cascade up the chain to the root workflow
  • Standalone Inquiries (not in a workflow). Not a common use case, but in previous discussions, we decided to support this. Confirm you can run core.ask on its own, and that it supports the same response behavior.
  • Send slack notifications using a rule that consumes the core.st2.generic.inquiry trigger. Note that this is just a PoC - a future PR will introduce proper chatops integration once this core functionality is vetted
  • Test all client commands and flags (st2 inquiry get/list/respond). Ensure the output is as you would expect
  • RBAC (This is my first time doing anything with RBAC so I would appreciate some eyes on this) - Ensure you can lock down Inquiry resources the way you'd expect from other resources. I played around with this a bit myself, feel free to use my example as a starting point (slightly out of date - no longer necessary to use inquiry:ask; just use inquiry). This was one area where not having a dedicated model for Inquiries made things difficult (though not impossible).
  • Response permissions using users or roles runner parameters. This goes beyond RBAC and actually permits/denies a per-Inquiry response

Again, please review the corresponding st2docs PR for full usage documentation and examples.

TODOs

  • Test RBAC manually to ensure things still work
  • (winson) Are there integration tests for the st2cd.st2_e2e_tests? Please include one for action chain and one for mistral. Also provide PR to st2cd (and st2ci?) to run this just like the mistral itests are being run
  • Finish docs
  • Check coverage
  • Ensure example chain and mistral workflow is solid

Matt Oswalt and others added 30 commits June 30, 2017 01:54
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
I would prefer that this is done using the API, which can be done with a Python action

Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <oswaltm@brocade.com>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
The from_dict function was too close to from_model; the only difference
was that it skipped the call to cls._from_model

So I just added a parameter to skip this, and in the Inquiries controller
I can just convert the InquiryAPI dicts directly into InquiryResponseAPI
instances, since the latter is more restrictive.

Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
PermissionType.INQUIRY_ALL
]
permission_grants = get_all_permission_grants_for_user(user_db=user_db,
resource_types=resource_types,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since no resource_id is passed here it looks like those permissions should be global (don't apply to a specific resource) if that's not already the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just saw the code below. I see the approach you went with.

Another approach would be to make those permission types global (don't apply to a specific resource), then you could get rid of that if len check below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was mostly because Inquiries are still just executions under the hood so I wasn't sure of a better way. I'll see if I can convert the permissions to a global type and get rid of this hack.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a stab at this in 7d828a2. It seems to work (at least the tests are happy, will do some additional testing later) and I'm very happy to s/inquiry:ask/inquiry everywhere but I'm not confident that I satisfied your statement: you could get rid of that if len check below. For now, I'm assuming by that you meant getting rid of the list comprehension, since there are other resolvers that do a len() check, just in a much simpler way (just checking total len of permission_grants). Let me know if I missed something there.

Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
…ies UID hack

Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Also fixed bug where the ID in the trigger payload was the ID of the Inquiry
liveaction instead of the ActionExecution. Added test for it

Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
@Mierdin
Copy link
Member Author

Mierdin commented Sep 28, 2017

Quick update on some somewhat major changes:

  • (in 545e3b6) The trigger definition for Inquiries now only includes the Inquiry ID. Also fixed a bug in that commit where the ID being placed in the trigger payload was accidentally the liveaction ID. Corrected this to use the ActionExecution ID (which is equal to the Inquiry ID), and fixed test to enforce this. EDIT meant to leave the route (formerly tag) field in there as well, as that's obviously needed for notification purposes. Added back in f5faf51
  • (in d17ad68) Renamed all instances of the tag parameter to route to keep things more consistent with notifications

Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
…roperly

Inquiries play a bit of surgery with the liveactions behind the scenes, so when an ActionChain
resumes from a Inquiry pause, it correctly retrieves an updated result when it detects a state
change it didn't previously know about.

However, it doesn't update context_result, which is what ultimately gets passed into the Jinja
renderer. So this makes sure the appropriate task results is updated in context_result

Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
Signed-off-by: Matt Oswalt <matt@keepingitclassless.net>
@Mierdin Mierdin merged commit fff02fc into master Oct 3, 2017
@arm4b
Copy link
Member

arm4b commented Oct 3, 2017

Congrats! 👏

@arm4b arm4b deleted the api-ask-response branch October 3, 2017 14:07
@cognifloyd cognifloyd removed the RFR label Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants